Added erf(x) Float64 and Float32 Julia implementations#491
Added erf(x) Float64 and Float32 Julia implementations#491oscardssmith merged 26 commits intoJuliaMath:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #491 +/- ##
==========================================
+ Coverage 94.02% 94.16% +0.13%
==========================================
Files 14 14
Lines 2897 2965 +68
==========================================
+ Hits 2724 2792 +68
Misses 173 173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Old:
Float32
New:
Float32
|
|
Float32 implementation available, but not faster than Float64 version due to a exp() call. |
|
need to clean up polynomial evaluations. |
|
Remaining: erfc Float64 and Float32 implementations, and the erf Float32 implementation |
…necessary whitespace, and removed explicit copysigns
|
|
||
| end | ||
|
|
||
| _erf(x::Float16)=Float16(_erf(Float32(x))) |
There was a problem hiding this comment.
if you wanted to do a Float16 impl, it should be easier than the others. Specifically, the domain is only to 2, and the accuracy required is much reduced.
There was a problem hiding this comment.
100% could wait for a followup PR.
There was a problem hiding this comment.
I'm thinking that too to be honest.
this and the poli regen.
|
Given that this is faster and accurate, seems good to merge to me! |
|
Are there any tests for edge cases/ULP in the c version we do not do ourselves? |
devmotion
left a comment
There was a problem hiding this comment.
The implementation does not handle NaN32 and NaN16 correctly:
julia> erf(NaN32)
1.0f0
julia> erf(NaN16)
Float16(1.0)Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
|
There aren't any tests for erfc. Is that expected? |
|
Any other changes needed? |
|
we should probably should test erfc. |
|
Other than missing tests for Inf, looks good to me. @devmotion any further sugestions? |
|
What do the benchmark shows with the latest iteration of this PR? In #491 (comment) performance with |
|
Yes, you're correct. The old implementation was not efficient so I redid it. OldFloat64
Float32
NewFloat64
Float32
|
devmotion
left a comment
There was a problem hiding this comment.
If you update the version number, we could tag a new release immediately after merging the PR.
|
Anything left for me to do? |
|
yeah, can you bump the patch number (in |
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
|
Thanks @AhmedYKadah for the Pr and @devmotion for the additional review! |
|
It's been a pleasure 😄 |
Faster than current wrapper function call (including Float32 function call).
Uses algorithm based on https://github.com/ARM-software/optimized-routines/blob/master/math/erf.c